Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: watch mode improvments #48259

Closed
wants to merge 2 commits into from
Closed

test_runner: watch mode improvments #48259

wants to merge 2 commits into from

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented May 30, 2023

some fixes for using run with watch: true:

  • emit an event when no more queued tests
  • the queued tests were in a global state preventing the event ^ from working correctly when using multiple run's
  • respect AbortSignal

@MoLow MoLow requested review from benjamingr and cjihrig May 30, 2023 23:58
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels May 30, 2023
@MoLow MoLow added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label May 31, 2023
cjihrig
cjihrig approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM with a few minor comments. I do have two questions:

  1. What was the motivation for this new event?
  2. Why do we only care about this event in the context of watch mode? Is there value in having a generic test:drain event?

doc/api/test.md Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
@MoLow
Copy link
Member Author

MoLow commented May 31, 2023

Code LGTM with a few minor comments. I do have two questions:

  1. What was the motivation for this new event?
  2. Why do we only care about this event in the context of watch mode? Is there value in having a generic test:drain event?

I have started experimenting with an npm package that runs tests in interactive mode. just a feedback/feature request from friends.
I don't think a generic test:drain is needed - since when not in watch mode you can simply listen to the testsStream end event

Screen.Recording.2023-05-31.at.11.13.02.mov

@MoLow MoLow force-pushed the test-runner-watch-mode-additions branch from 39bc086 to a52fb04 Compare May 31, 2023 09:08
benjamingr
benjamingr reviewed May 31, 2023
View reviewed changes
lib/internal/test_runner/runner.js Outdated
@@ -432,6 +442,11 @@ function watchFiles(testFiles, root, inspectPort, testNamePatterns) {
triggerUncaughtException(error, true /* fromPromise */);
}));
});
signal?.addEventListener('abort', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to remove this listener if abort never happens but the watcher is already cleared or is this always before the process dies and "hanging" this if we never abort is fine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is ok - if you don't pass a signal the watcher will never clear/close

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concern was about leaking the listener on the signal after run finished.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed - in watch mode run does not finish besides this new flow

@MoLow MoLow added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52014/

@MoLow MoLow force-pushed the test-runner-watch-mode-additions branch from a52fb04 to d55ee1e Compare May 31, 2023 21:00
@MoLow
Copy link
Member Author

MoLow commented May 31, 2023 •
edited
Loading

@benjamingr I pushed a fix for a race condition (the test finishes before the watch mode ipc message is processed) - so the abortSignal is now passed into fs.watch
PTAL (and re-approve for convenience of the commit queue)

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52036/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52041/

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@github-actions github-actions bot mentioned this pull request Jun 2, 2023
Open
34 tasks
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 2, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 0cf8bbe...5d685e4

nodejs-github-bot pushed a commit that referenced this pull request Jun 2, 2023
@MoLow @nodejs-github-bot
test_runner: stop watch mode when abortSignal aborted
ca71ae6 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jun 2, 2023
@MoLow @nodejs-github-bot
test_runner: emit test:watch:drained event
5d685e4 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@github-actions github-actions bot mentioned this pull request Jun 3, 2023
Open
33 tasks
@MoLow MoLow deleted the test-runner-watch-mode-additions branch June 3, 2023 19:25
@github-actions github-actions bot mentioned this pull request Jun 4, 2023
Open
30 tasks
targos pushed a commit that referenced this pull request Jun 4, 2023
@MoLow @targos
test_runner: stop watch mode when abortSignal aborted
c9f8e8c 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Jun 4, 2023
@MoLow @targos
test_runner: emit test:watch:drained event
3f259b7 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Jun 4, 2023
Merged
This was referenced Jun 5, 2023
Open
Open
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
@MoLow @franciszek-koltuniuk-red
test_runner: stop watch mode when abortSignal aborted
37810a6 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
@MoLow @franciszek-koltuniuk-red
test_runner: emit test:watch:drained event
53af71e 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@MoLow @danielleadams
test_runner: stop watch mode when abortSignal aborted
97aad16 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@MoLow @danielleadams
test_runner: emit test:watch:drained event
68f2c49 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
@MoLow
test_runner: stop watch mode when abortSignal aborted
618a9e1 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
@MoLow
test_runner: emit test:watch:drained event
033d0bb 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
@philnash philnash mentioned this pull request Aug 2, 2023
Merged
7 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
test_runner: stop watch mode when abortSignal aborted
40a500d 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
test_runner: emit test:watch:drained event
6ba2949 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
test_runner: stop watch mode when abortSignal aborted
7082c47 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
test_runner: emit test:watch:drained event
a27c30f 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MoLow @nodejs-github-bot @benjamingr @cjihrig